Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/yaml config #1401

Merged
merged 12 commits into from
Dec 11, 2024
Merged

Feature/yaml config #1401

merged 12 commits into from
Dec 11, 2024

Conversation

neolynx
Copy link
Member

@neolynx neolynx commented Dec 2, 2024

Description of the Change

Documentation of the aptly configuration should be inline, which can be done with json:

In addition to json format config file, aptly can also support yaml format, which is easier to document:

This change brings the following:

  • default configuration is documented yaml (created in home when no config file exists)
  • /etc/aptly.conf in aptly-api debian package is documented yaml
  • zaml file contains default values, and example sections to be uncommented
  • existing json configuration files are still valid
  • json configuration can be converted to yaml with aptly config show -yaml (without documentation)

Checklist

  • man page updated (if applicable)
  • documentation updated

@neolynx neolynx changed the base branch from master to improve/documentation December 2, 2024 20:31
@neolynx neolynx force-pushed the feature/yaml-config branch from f585820 to 8a9284e Compare December 2, 2024 20:33
@neolynx neolynx added the WIP label Dec 2, 2024
@neolynx neolynx self-assigned this Dec 2, 2024
@neolynx neolynx force-pushed the improve/documentation branch from cd7b973 to 8cf1e31 Compare December 2, 2024 21:19
@neolynx neolynx force-pushed the feature/yaml-config branch 6 times, most recently from 9190008 to 5342e79 Compare December 2, 2024 22:31
@neolynx neolynx added needs review Ready for review & merge and removed WIP labels Dec 2, 2024
@neolynx neolynx requested a review from a team December 2, 2024 22:46
debian/aptly.conf Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 77.14286% with 16 lines in your changes missing coverage. Please review.

Project coverage is 75.02%. Comparing base (465312b) to head (0d90ff9).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
utils/config.go 82.14% 7 Missing and 3 partials ⚠️
cmd/config_show.go 57.14% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1401      +/-   ##
==========================================
- Coverage   75.02%   75.02%   -0.01%     
==========================================
  Files         157      157              
  Lines       18146    18206      +60     
==========================================
+ Hits        13614    13659      +45     
- Misses       3409     3420      +11     
- Partials     1123     1127       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@neolynx neolynx added the increase coverage The PR lacks test coverage label Dec 2, 2024
@iofq
Copy link
Contributor

iofq commented Dec 3, 2024

What is your reasoning for having new different keys in yaml for e.g rootDir vs root_dir? While i appreciate that yaml will be the default moving forward, having two different keys can cause confusion and I see little motivation for making a breaking change here

@neolynx neolynx force-pushed the improve/documentation branch from 5a60298 to 0b35072 Compare December 3, 2024 11:12
@neolynx neolynx force-pushed the feature/yaml-config branch from 1978ef5 to b2c49ab Compare December 3, 2024 11:20
@neolynx neolynx force-pushed the improve/documentation branch from 4063016 to 22cd4e2 Compare December 3, 2024 11:25
@neolynx neolynx force-pushed the feature/yaml-config branch 2 times, most recently from 108b217 to 5498e84 Compare December 3, 2024 12:33
@neolynx
Copy link
Member Author

neolynx commented Dec 3, 2024

I was using the same syntax as in json, then I realized that snake case is recommended.
Since aptly condif show -yaml will convert existing json config, maybe it is not such a big migration (albeit the documentation comments will be missing). Also, the migration is optional, the json config is still valid and used by aptly.

we can still go with the json syntax if that is preferred.

@neolynx neolynx force-pushed the feature/yaml-config branch 2 times, most recently from 220448e to 8377147 Compare December 3, 2024 17:01
@neolynx neolynx removed the increase coverage The PR lacks test coverage label Dec 3, 2024
@neolynx neolynx force-pushed the feature/yaml-config branch 2 times, most recently from 50d54d4 to be4bb6a Compare December 4, 2024 12:07
@neolynx
Copy link
Member Author

neolynx commented Dec 4, 2024

Please vote for default (newly created) aptly config file to be:

  • 👍: yaml with snake_case (root_dir, current)
  • 🎉: yaml with same fields as json config (rootDir)
  • 🚀: json with documentation comments
  • 😕: stay with current json config, undocumented

@szakalboss
Copy link

snake case for consistency; btw snake type is python friendly ;)

@neolynx neolynx force-pushed the feature/yaml-config branch 2 times, most recently from 32b24bb to ea9fc94 Compare December 4, 2024 12:52
@neolynx neolynx force-pushed the improve/documentation branch from da589f2 to 067e2a1 Compare December 4, 2024 21:35
@neolynx neolynx force-pushed the feature/yaml-config branch from ea9fc94 to 06e384c Compare December 4, 2024 21:36
@neolynx neolynx force-pushed the improve/documentation branch from 067e2a1 to e319f3c Compare December 11, 2024 10:20
Base automatically changed from improve/documentation to master December 11, 2024 11:02
@neolynx neolynx force-pushed the feature/yaml-config branch from 3cb7da4 to 0d90ff9 Compare December 11, 2024 11:03
@neolynx neolynx merged commit d873278 into master Dec 11, 2024
46 checks passed
@neolynx neolynx deleted the feature/yaml-config branch December 11, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs approval needs review Ready for review & merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants